add getConnections method and deprecate getSkeleton for bodyPose
#227
Conversation
|
This is great! Before merging I just want to check with @MOQN @gohai and @jackbdu who are teaching with
I'd like to merge this relatively soon so that we can get it into an upcoming release. Thank you! |
|
Thank you - this looks great.
My only comment would be about „depreciated and will be removed in the
future“:
I don’t know if we /really/ want to take the old method away in the future,
especially since it such an uncomplicated mapping, but if we wanted to: how
about a warning to the console (printed once per program invocation) „
getSkeleton will be removed from ml5 releases after April 2026 - use
getConnections() instead“?
|
|
Thank you @gohai, I think that's a great suggestion! I think the idea of deprecated is that it would never be removed until we have a major release (2.0) at which time we might introduce breaking changes. I'm also ok with keeping both "forever" and having |
|
That makes a lot of sense. Since it is in materials, should we maybe call
it an „alias“ for the other function - and possibly even keep it in the
reference with a link?
|
|
This is great! Thank you. We will update the website once it's applied to the library! |
|
All looks great to me! Looking forward to the next release. |
src/BodyPose/index.js
Outdated
| * An alias for `getConnections`. This method is deprecated and will be removed in the future. | ||
| * @returns {number[][]} an array of pairs of indices containing the connected keypoints. | ||
| * @public | ||
| * @deprecated |
There was a problem hiding this comment.
One last little detail here. Following up on my discussion with @gohai, would it make sense to take out "will be removed in the future" and well as "@deprecated" and just leave both methods in, one as an alias? Or is that confusing / bad practice? Would this make the documentation confusing to have two methods that do the same thing? cc @MOQN @QuinnHe @alanvww
We can always change our mind later, we don't have to feel this is set in stone.
There was a problem hiding this comment.
I can take it out for now!
|
This had me really ponder; please allow me to share my humble thoughts. 🙏 I initially thought that it would be best if the alias were deprecated and removed in the future to minimize confusion. I really like This is based on my experience with other motion capture systems; I mostly used the term 'skeleton' rather than 'connections' while using their SDKs and relevant libraries. I assume that some users and our students might search for references using the keyword 'skeleton.' So, in this case, it would be great if they could find |
|
Thank you @MOQN for your thoughts! This is good to go! Merging! I believe this is the last piece of the puzzle for the next release to incorporate these new methods and data structures! |
This PR adds the
getConnectionsmethod tobodyPoseand deprecates the originalgetSkeletonmethod forbodyPose. The two methods are functionally identical.getConnectionsmethod and makegetSkeletonan aliasgetSkeletonas deprecatedgetConnectionsinstead ofgetSkeleton